Creating PR with the latest changes #92
Conversation
| @@ -116,18 +117,20 @@ def upload_file(file_path, options, uploaded=None, failed=None): | |||
| verbose = logger.getEffectiveLevel() < logging.INFO | |||
|
|
|||
| try: | |||
| size = path.getsize(file_path) | |||
There was a problem hiding this comment.
We can change this to:
size = 0 if cloudinary.utils.is_remote_url(file_path) else path.getsize(file_path)
cloudinary_cli/utils/api_utils.py
Outdated
| result = upload_func(file_path, **options) | ||
| disp_path = _display_path(result) | ||
| disp_str = f"as {result['public_id']}" if not disp_path \ | ||
| else f"as {disp_path} with public_id: {result['public_id']}" | ||
| logger.info(style(f"Successfully uploaded {file_path} {disp_str}", fg="green")) | ||
| if verbose: | ||
| print_json(result) | ||
| uploaded[file_path] = {"path": asset_source(result), "display_path": disp_path} | ||
| if "async" not in options: |
There was a problem hiding this comment.
That is a good catch!
I would change line 127 to:
if "batch_id" in result:
disp_str = f"asynchnously with batch_id: {result['batch_id']}"
else:
disp_str = f"as {result['public_id']}" if not disp_path \
else f"as {disp_path} with public_id: {result['public_id']}"
then you can remove this line (if "async" not in options:)
and in
def asset_source(asset_details):
change to
base_name = asset_details.get('public_id', '')
if not base_name:
return base_name
There was a problem hiding this comment.
This is done.
I had to also update the last line to asset_details.get('format', '') as non-raw files still get the public_id as we specify them so this will prevent no format error as the response won't contain the format
Finally, I updated the display message to say it's processing when uploading asynchronously as well.
cloudinary_cli/modules/clone.py
Outdated
| You need to specify the target cloud via `-t` or `-T` (not both) | ||
| e.g. cld clone -t cloudinary://<api_key>:<api_secret>@<cloudname> -f tags,context -O | ||
| """) | ||
| @option("-T", "--target_saved", |
There was a problem hiding this comment.
We can combine -Tand -t into one. it's easy to check if it's a saved config, or a cloudinary url or not both.
cloudinary_cli/modules/clone.py
Outdated
| help="Tell the CLI the target environemnt to run the command on by specifying a saved configuration - see `config` command.") | ||
| @option("-t", "--target", | ||
| help="Tell the CLI the target environemnt to run the command on by specifying an environment variable.") | ||
| @option("-A", "--auto_paginate", is_flag=True, default=False, |
There was a problem hiding this comment.
This can be removed
cloudinary_cli/modules/clone.py
Outdated
| @option("-F", "--force", is_flag=True, | ||
| help="Skip confirmation.") | ||
| @option("-O", "--overwrite", is_flag=True, default=False, | ||
| help="Skip confirmation.") |
There was a problem hiding this comment.
Doc string seems to be invalid
| help="Specify whether to copy tags and context.") | ||
| @option("-se", "--search_exp", default="", | ||
| help="Define a search expression.") | ||
| @option("--async", "async_", is_flag=True, default=False, |
There was a problem hiding this comment.
"async_" seems to be redundant, you can just specify one ("--async") option
cloudinary_cli/modules/clone.py
Outdated
| print_help_and_exit() | ||
|
|
||
| base_cloudname_url = os.environ.get('CLOUDINARY_URL') | ||
| base_cloudname = cloudinary.config().cloud_name |
There was a problem hiding this comment.
I would rename base_cloudname to source_cloud_name
cloudinary_cli/modules/clone.py
Outdated
| logger.info("Target environment cannot be the " | ||
| "same as source environment.") | ||
| return True | ||
| refresh_config(base_cloudname_url) |
There was a problem hiding this comment.
Instead of loading target, and then resetting, you can just create a new instance of cloudinary.Config() class and use it.
cloudinary_cli/modules/clone.py
Outdated
| 'secure_url', 'display_name']) | ||
| search.max_results(DEFAULT_MAX_RESULTS) | ||
| res = execute_single_request(search, fields_to_keep="") | ||
| if auto_paginate: |
There was a problem hiding this comment.
auto paginate looks redundant
| if "context" in copy_fields: | ||
| cloned_options['context'] = res.get('context') | ||
| if res.get('folder'): | ||
| cloned_options['asset_folder'] = res.get('folder') |
There was a problem hiding this comment.
| cloned_options['asset_folder'] = res.get('folder') | |
| cloned_options['folder'] = res.get('folder') |
There was a problem hiding this comment.
so need cloned_options['asset_folder'] = res.get('folder') to put the asset in the right asset folder if the user is copying from fixed to dynamic. If you pass a folder to a dynamic folder account then it will append this to the public_id as well and we don't want this.
There was a problem hiding this comment.
I would put a comment explaining that
Brief Summary of Changes
What does this PR address?
Are tests included?
Reviewer, please note:
Checklist: